Enable mode-specific transfers by storing mode information in transfers #6293

VillePihlava · 2024-11-29T13:57:05Z

Summary

This PR adds StreetMode information to transfers. This enables using mode-specific transfers during runtime.

Notes:

Instead of a collection of PathTransfers with mode information, this could also be done by using mappings from modes to transfers.
- Depending on how this is done, this could maybe be cheaper in terms of memory?
- In my tests the size of the graph has a similar size with both implementations. I prefer the implementation in this PR
The size of the graph can increase a lot when using a large dataset and when using CAR transfers.
- When I tested building the graph with data from almost the entirety of Finland, the size of the graph file increased from 1.4G to 6.9G.
- When using car transfers, because the transfers are calculated based on a range by duration there can be a lot of transfers.
- The solution is to use CAR transfers only with relevant stops.
- I will create a PR in the future for optionally limiting which stops can use transfers with cars.
- Using an EnumSet was the cheapest way I could think of adding this functionality when having the information in PathTransfer.
For large datasets with many transfers, build duration can increase.
- When I tested building the graph with data from almost the entirety of Finland, build duration doubled.
- For less transfers, build duration does not increase as much.

Issue

Resolves a TODO comment about adding StreetMode information in the PathTransfer class.

Related to #5875

Unit tests

Added a new test to DirectTransferGeneratorTest.

Documentation

No documentation has been added.

Bumping the serialization version id

This changes how transfers are generated during the graph build.

…ic transfers.

VillePihlava · 2024-11-29T13:59:15Z

The tests will fail for CarPickupSnapshotTest. There is discussion related to changing the test in #6238. This PR should not be merged before that test has been changed and the tests pass.

VillePihlava · 2024-11-29T14:08:38Z

Interestingly FlexIntegrationTest fails as well. I was able to locally run this test and all other tests except CarPickupSnapshotTest successfully, but I have noticed some flakiness related to running that test and running tests in general.

VillePihlava · 2024-11-29T14:33:13Z

Interestingly now after making changes not related to functionality, the FlexIntegrationTest did not fail.

…into split-transfers-by-mode-pathtransfer-mode

…er requests for WALK mode.

codecov · 2024-12-04T08:49:20Z

Codecov Report

Attention: Patch coverage is 78.48101% with 17 lines in your changes missing coverage. Please review.

Project coverage is 69.82%. Comparing base (69e6f64) to head (18215a9).
Report is 181 commits behind head on dev-2.x.

Files with missing lines	Patch %	Lines
...apping/StreetModeToTransferTraverseModeMapper.java	14.28%	5 Missing and 1 partial ⚠️
...pentripplanner/api/parameter/QualifiedModeSet.java	0.00%	3 Missing ⚠️
.../graph_builder/module/DirectTransferGenerator.java	91.17%	1 Missing and 2 partials ⚠️
...raptoradapter/transit/mappers/TransfersMapper.java	71.42%	2 Missing ⚠️
...t/java/org/opentripplanner/ext/flex/FlexIndex.java	75.00%	1 Missing ⚠️
.../java/org/opentripplanner/ext/flex/FlexRouter.java	0.00%	1 Missing ⚠️
...n/java/org/opentripplanner/model/PathTransfer.java	87.50%	1 Missing ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             dev-2.x    #6293      +/-   ##
=============================================
+ Coverage      69.79%   69.82%   +0.03%     
- Complexity     17788    17842      +54     
=============================================
  Files           2017     2022       +5     
  Lines          76042    76303     +261     
  Branches        7781     7804      +23     
=============================================
+ Hits           53074    53281     +207     
- Misses         20264    20306      +42     
- Partials        2704     2716      +12

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

optionsome · 2024-12-05T12:23:03Z

application/src/main/java/org/opentripplanner/graph_builder/module/DirectTransferGenerator.java

+            if (pathTransfer == null) {
+              EnumSet<StreetMode> modes = EnumSet.of(mode);
+              distinctTransfers.put(
+                transferKey,
+                new PathTransfer(stop, sd.stop, sd.distance, sd.edges, modes)
+              );
+            } else {
+              pathTransfer.addMode(mode);
+            }


You could add a comment here to explain what these different cases mean.

I added comments for the different cases

optionsome · 2024-12-05T12:29:17Z

application/src/main/java/org/opentripplanner/graph_builder/module/DirectTransferGenerator.java

+        }
+        // Calculate flex transfers if flex routing is enabled.
+        for (RouteRequest transferProfile : flexTransferRequests) {
+          StreetMode mode = transferProfile.journey().transfer().mode();


Is this just always WALK?

We have always assumed that flex transfers can only be on foot. I think this is reasonable.

I changed this to StreetMode mode = StreetMode.WALK;

optionsome · 2024-12-05T12:41:22Z

...tion/src/test/java/org/opentripplanner/graph_builder/module/DirectTransferGeneratorTest.java

@@ -250,6 +250,61 @@ public void testMultipleRequestsWithPatterns() {
    );
  }

+  @Test
+  public void testPathTransfersWithModesForMultipleRequestsWithPatterns() {


What does multiple requests with patterns mean?

I tried to follow a similar naming convention as the other tests in the file. With patterns means that trippatterns are added to the timetablerepository of the test. Multiple requests means that transfers are generated for multiple requests that have different modes:

var reqWalk = new RouteRequest(); reqWalk.journey().transfer().setMode(StreetMode.WALK); var reqBike = new RouteRequest(); reqBike.journey().transfer().setMode(StreetMode.BIKE); var transferRequests = List.of(reqWalk, reqBike);

leonardehrenfried · 2024-12-06T12:31:40Z

...tion/src/main/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/Transfer.java

    this.toStop = toStop;
    this.edges = edges;
    this.distanceMeters = (int) edges.stream().mapToDouble(Edge::getDistanceMeters).sum();
+    this.modes = modes;


We probably want to make this immutable. EnumSet are not.

I changed the getModes method to clone the enum set

leonardehrenfried · 2024-12-06T12:32:30Z

...tion/src/test/java/org/opentripplanner/graph_builder/module/DirectTransferGeneratorTest.java

+    var walkTransfers = timetableRepository
+      .getAllPathTransfers()
+      .stream()
+      .filter(pathTransfer -> pathTransfer.getModes().contains(StreetMode.WALK))
+      .toList();
+    var bikeTransfers = timetableRepository
+      .getAllPathTransfers()
+      .stream()
+      .filter(pathTransfer -> pathTransfer.getModes().contains(StreetMode.BIKE))
+      .toList();
+    var carTransfers = timetableRepository
+      .getAllPathTransfers()
+      .stream()
+      .filter(pathTransfer -> pathTransfer.getModes().contains(StreetMode.CAR))
+      .toList();


please extract a method

I created a getTransfersByMode method in TimetableRepository

leonardehrenfried · 2024-12-06T12:36:05Z

The solution is to use CAR transfers only with relevant stops.

Does this mean that this PR allows you to compute CAR transfers between stops that don't serve cars at all?

leonardehrenfried · 2024-12-06T12:39:50Z

...in/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/RaptorTransferIndex.java

@@ -41,6 +43,7 @@ public static RaptorTransferIndex create(
      var transfers = transfersByStopIndex
        .get(fromStop)
        .stream()
+        .filter(transfer -> transfer.getModes().contains(mode))


This is run only once and then cached for the specific combination of request parameters, correct?

Yes, this is run once for each cache request. You can see the code that uses this in RaptorRequestTransferCache

leonardehrenfried · 2024-12-06T12:50:17Z

application/src/main/java/org/opentripplanner/model/PathTransfer.java

+    return this.modes;
+  }
+
+  public boolean addMode(StreetMode mode) {


I don't think we want to make this class mutable. Can you create a copy method, for example withAddedMode that returns a new instance instead?

I changed this to use the withAddedMode method

leonardehrenfried · 2024-12-06T12:51:02Z

application/src/main/java/org/opentripplanner/model/PathTransfer.java

@@ -46,6 +56,14 @@ public List<Edge> getEdges() {
    return this.edges;
  }

+  public EnumSet<StreetMode> getModes() {


We don't want to expose the mutable EnumSet. Can you replace it with a method like allows(CAR)?

I changed the getModes method to clone the enum set

application/src/ext/java/org/opentripplanner/ext/flex/FlexIndex.java

leonardehrenfried · 2024-12-06T13:03:44Z

application/src/main/java/org/opentripplanner/graph_builder/module/DirectTransferGenerator.java

+      .map(transferProfile -> transferProfile.journey().transfer().mode())
+      .collect(Collectors.toSet())) {


Suggested change

.map(transferProfile -> transferProfile.journey().transfer().mode())

.collect(Collectors.toSet())) {

.map(transferProfile -> transferProfile.journey().transfer().mode())

.distinct()

.toList()

Not a huge deal but avoiding a set gives you predictable sort order.

Thanks, this is a good suggestion. I changed it to this

VillePihlava · 2024-12-09T11:58:40Z

Does this mean that this PR allows you to compute CAR transfers between stops that don't serve cars at all?

Yes, in a future PR (#6215) I will implement more configurability for transfers

leonardehrenfried · 2024-12-09T12:03:51Z

application/src/main/java/org/opentripplanner/model/PathTransfer.java

+  }
+
+  public EnumSet<StreetMode> getModes() {
+    return modes.clone();


Suggested change

return modes.clone();

return EnumSet.copyOf(modes);

I changed this

leonardehrenfried · 2024-12-09T12:08:58Z

...tion/src/main/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/Transfer.java

    this.toStop = toStopIndex;
    this.distanceMeters = distanceMeters;
    this.edges = null;
+    this.modes = modes;


This transfer is not serialized. Here you can use Sets.immutableEnumSet and then never have to worry about cloning or copying.

I changed this to use Sets.immutableEnumSet

optionsome · 2024-12-10T14:33:53Z

application/src/main/java/org/opentripplanner/graph_builder/module/DirectTransferGenerator.java

+    for (StreetMode mode : transferRequests
+      .stream()
+      .map(transferProfile -> transferProfile.journey().transfer().mode())
+      .distinct()
+      .toList()) {
+      LOG.info(
+        "Created {} transfers for mode {}.",
+        transfersByStop
+          .values()
+          .stream()
+          .filter(pathTransfer -> pathTransfer.getModes().contains(mode))
+          .count(),
+        mode
+      );


This is quite difficult to read. I would probably continue the stream instead of making it a list and then looping over it, and then call some method with the mode as an argument that does the logging. So something like:

transferRequests .stream() .map(transferProfile -> transferProfile.journey().transfer().mode()) .distinct() .peek(mode -> logTransfersForMode(mode));

I changed it to this:

transferRequests .stream() .map(transferProfile -> transferProfile.journey().transfer().mode()) .distinct() .forEach(mode -> LOG.info( "Created {} transfers for mode {}.", timetableRepository.getTransfersByMode(mode).size(), mode ) );

miklcct · 2024-12-10T16:24:22Z

I am following this as I am now testing BICYCLE routing on our deployment. The current deployment configuration only has "walk" with PT15M in build-config.json and the bicycle routing is near-useless for cross-London journeys, as it can't return results cycling from one London terminal to another (some of these transfers require about 30 minutes on the bike).

However, if I set the build-config.json to do walk and bike transfers with PT30M duration, the graph has become 15.1 GB just for London only without any other parts of Great Britain, and the transfer cache takes forever to build rather than seconds that it can't even cope with a city-sized deployment, and I can't even think of how it can work when put on a country-wide deployment.

optionsome · 2024-12-12T11:14:49Z

application/src/main/java/org/opentripplanner/transit/service/TimetableRepository.java

@@ -434,7 +435,7 @@ public Collection<PathTransfer> getTransfersByStop(StopLocation stop) {
    return transfersByStop.get(stop);
  }

-  public Collection<PathTransfer> getTransfersByMode(StreetMode mode) {
+  public List<PathTransfer> getTransfersByMode(StreetMode mode) {


I think this method should be called findTransfers according to the naming conventions.

I renamed getTransfersByMode to findTransfers

leonardehrenfried · 2024-12-12T12:24:45Z

@t2gran can we run the speed tests against that? Do you know how to do it?

leonardehrenfried · 2024-12-13T12:31:38Z

...tion/src/main/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/Transfer.java

@@ -31,16 +35,20 @@ public class Transfer {

  private final List<Edge> edges;

-  public Transfer(int toStop, List<Edge> edges) {
+  private final ImmutableSet<StreetMode> modes;


I know there was a lot of back and forth with this. In the end @t2gran says he didn't want Guava collection, so lets use Collections.unmodifiableSet.

Suggested change

private final ImmutableSet<StreetMode> modes;

private final Set<StreetMode> modes;

The reason we can do it here is that it's not serialized.

I changed it to this

leonardehrenfried · 2024-12-13T12:32:10Z

...tion/src/main/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/Transfer.java

    this.toStop = toStop;
    this.edges = edges;
    this.distanceMeters = (int) edges.stream().mapToDouble(Edge::getDistanceMeters).sum();
+    this.modes = Sets.immutableEnumSet(modes);


Suggested change

this.modes = Sets.immutableEnumSet(modes);

this.modes = Collections.unmodifiableSet(modes);

I changed it to this

leonardehrenfried · 2024-12-13T12:32:42Z

...tion/src/main/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/Transfer.java

    this.toStop = toStopIndex;
    this.distanceMeters = distanceMeters;
    this.edges = null;
+    this.modes = Sets.immutableEnumSet(modes);


Suggested change

this.modes = Sets.immutableEnumSet(modes);

this.modes = Collections.unmodifiableSet(modes);

I changed it to this

leonardehrenfried · 2024-12-13T12:44:33Z

...tion/src/main/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/Transfer.java

@@ -68,6 +76,10 @@ public List<Edge> getEdges() {
    return edges;
  }

+  public ImmutableSet<StreetMode> getModes() {


You don't actually need to expose the collection at all. Simply add a method allowsMode(mode). So you don't even have to think about if it's mutable or immutable.

Also document the method.

I changed getModes to allowsMode

leonardehrenfried · 2024-12-13T13:30:32Z

application/src/main/java/org/opentripplanner/transit/service/TimetableRepository.java

@@ -433,6 +435,14 @@ public Collection<PathTransfer> getTransfersByStop(StopLocation stop) {
    return transfersByStop.get(stop);
  }

+  public List<PathTransfer> findTransfers(StreetMode mode) {


Add Javadoc here.

leonardehrenfried · 2024-12-13T13:31:12Z

...va/org/opentripplanner/routing/algorithm/mapping/StreetModeToTransferTraverseModeMapper.java

+/**
+ * Maps street mode to transfer traverse mode.
+ */
+public class StreetModeToTransferTraverseModeMapper {


Nice that you extraced the mapper.

leonardehrenfried · 2024-12-13T13:48:38Z

I also ran the speed test on this branch and I cannot see any changes: https://tinyurl.com/27zv22uz

Performance-wise this is fine.

leonardehrenfried · 2024-12-13T14:06:44Z

application/src/main/java/org/opentripplanner/model/PathTransfer.java

+    return EnumSet.copyOf(modes);
+  }
+
+  public PathTransfer withAddedMode(StreetMode mode) {


Javadoc please.

leonardehrenfried · 2024-12-13T14:20:10Z

...tion/src/main/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/Transfer.java

@@ -68,6 +75,10 @@ public List<Edge> getEdges() {
    return edges;
  }

+  public boolean allowsMode(StreetMode mode) {


Javadoc here and then we are good.

I'll fix this next week. Btw does this need the bump serialization id label?

Yes. You are storing a new field into the graph.

This now has a comment

VillePihlava added 4 commits November 29, 2024 15:25

Rename 'transferByStopIndex' variable to 'transfersByStopIndex'.

33189c2

Add changes for CAR transfers.

aa4927e

Add EnumSet of StreetModes to PathTransfers and implement mode-specif…

4c882f4

…ic transfers.

Add test for mode-specific transfers.

44e4d31

VillePihlava requested a review from a team as a code owner November 29, 2024 13:57

Remove unused imports and small changes.

8448e6e

Add logging for mode-specific transfers.

3c46be3

habrahamsson-skanetrafiken requested a review from optionsome December 3, 2024 10:30

VillePihlava added 2 commits December 4, 2024 07:14

Merge branch 'dev-2.x' of github.com:opentripplanner/OpenTripPlanner …

c4951ff

…into split-transfers-by-mode-pathtransfer-mode

Fix flex routing transfer calculations and only calculate flex transf…

244c537

…er requests for WALK mode.

optionsome reviewed Dec 5, 2024

View reviewed changes

leonardehrenfried reviewed Dec 6, 2024

View reviewed changes

application/src/ext/java/org/opentripplanner/ext/flex/FlexIndex.java Show resolved Hide resolved

leonardehrenfried reviewed Dec 6, 2024

View reviewed changes

Add changes based on review comments.

012f149

leonardehrenfried reviewed Dec 9, 2024

View reviewed changes

Change how EnumSet is copied and make EnumSet immutable in Transfer.

984d34e

optionsome reviewed Dec 10, 2024

View reviewed changes

VillePihlava requested review from optionsome and leonardehrenfried December 11, 2024 14:51

Refactor to use method.

4a01328

optionsome reviewed Dec 12, 2024

View reviewed changes

Rename getTransfersByMode to findTransfers.

95e9ca7

optionsome previously approved these changes Dec 12, 2024

View reviewed changes

leonardehrenfried reviewed Dec 13, 2024

View reviewed changes

Changes based on review comments and small optimization.

df64c5a

VillePihlava dismissed optionsome’s stale review via df64c5a December 13, 2024 14:03

leonardehrenfried reviewed Dec 13, 2024

View reviewed changes

VillePihlava added 2 commits December 13, 2024 16:12

Add comments.

9b44df0

Fix spelling.

c75b0a2

leonardehrenfried reviewed Dec 13, 2024

View reviewed changes

leonardehrenfried added the bump serialization id Add this label if you want the serialization id automatically bumped after merging the PR label Dec 13, 2024

Add comment.

18215a9

leonardehrenfried approved these changes Dec 16, 2024

View reviewed changes

optionsome added the Improvement label Dec 16, 2024

optionsome added this to the 2.7 (next release) milestone Dec 16, 2024

optionsome approved these changes Dec 16, 2024

View reviewed changes

optionsome merged commit 19c2804 into opentripplanner:dev-2.x Dec 16, 2024
5 checks passed

optionsome deleted the split-transfers-by-mode-pathtransfer-mode branch December 16, 2024 11:49

t2gran pushed a commit that referenced this pull request Dec 16, 2024

Add changelog entry for #6293 [ci skip]

4375d3c

t2gran pushed a commit that referenced this pull request Dec 16, 2024

Bump serialization version id for #6293

74f2f22

		.map(transferProfile -> transferProfile.journey().transfer().mode())
		.collect(Collectors.toSet())) {

	private final ImmutableSet<StreetMode> modes;
	private final Set<StreetMode> modes;

	this.modes = Sets.immutableEnumSet(modes);
	this.modes = Collections.unmodifiableSet(modes);

Enable mode-specific transfers by storing mode information in transfers #6293

Enable mode-specific transfers by storing mode information in transfers #6293

Conversation

VillePihlava commented Nov 29, 2024 • edited Loading

Summary

Issue

Unit tests

Documentation

Bumping the serialization version id

VillePihlava commented Nov 29, 2024

VillePihlava commented Nov 29, 2024

VillePihlava commented Nov 29, 2024

codecov bot commented Dec 4, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leonardehrenfried commented Dec 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VillePihlava commented Dec 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VillePihlava Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

optionsome Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miklcct commented Dec 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leonardehrenfried commented Dec 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leonardehrenfried commented Dec 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VillePihlava commented Nov 29, 2024 •

edited

Loading

codecov bot commented Dec 4, 2024 •

edited

Loading

VillePihlava Dec 9, 2024 •

edited

Loading

optionsome Dec 10, 2024 •

edited

Loading